fix(server-elysia): update test mocks to match refactored http.createServer (#1204)#1210
fix(server-elysia): update test mocks to match refactored http.createServer (#1204)#1210kagura-agent wants to merge 4 commits intoVoltAgent:mainfrom
Conversation
…oltAgent#1206) Previously isDevRequest() treated any non-production NODE_ENV (including undefined) as a development environment, allowing auth bypass with a simple header. Deployed servers that forgot NODE_ENV=production were fully open. Now only NODE_ENV=development or NODE_ENV=test enable the dev bypass (fail-closed). Undefined/empty NODE_ENV is treated as production.
…ment helper Address CodeRabbit review: WebSocket auth functions in setup.ts also used the vulnerable NODE_ENV !== 'production' check. Extract isDevEnvironment() as a shared helper used by both HTTP and WebSocket auth paths. Also fix test names (empty vs undefined) and add isDevEnvironment unit tests.
…Server (VoltAgent#1204) Tests were mocking the old app.listen() API, but startServer() was refactored to use Node.js http.createServer() + server.listen(). Updated mocks to target node:http module instead, restoring test coverage for all 6 test cases.
🦋 Changeset detectedLatest commit: bcb54c8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughIntroduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
packages/server-elysia/src/elysia-server-provider.spec.ts (2)
22-32: Module-levelmockServeris shared across tests.Since
mockServeris defined at module scope andvi.clearAllMocks()only resets call history (not implementations or properties), state likelistening: truepersists across tests. The currentbeforeEachre-applies implementations forlisten/close/once, which is sufficient for the present test set, but any future test that mutatesmockServer.listening(or adds new methods) will leak into other tests. Consider resettingmockServerproperties explicitly inbeforeEachto make the isolation boundary obvious.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server-elysia/src/elysia-server-provider.spec.ts` around lines 22 - 32, The module-level mockServer object is shared across tests causing state leakage; in the beforeEach where you reapply implementations for listen/close/once, also explicitly reset mockServer properties (e.g., set mockServer.listening = false or true as appropriate and reinitialize any added methods) so each test starts with a fresh known state; update the beforeEach to reassign mockServer.listening and any other mutable fields on the mockServer used by tests (while keeping vi.mock("node:http") and the mocked listen/close/once implementations intact).
139-160: Startup error simulation relies on registration order — worth a brief comment.The test works because the implementation calls
server.once("error", reject)beforeserver.listen(...)(seeelysia-server-provider.ts:87-91), soerrorHandleris captured beforemockServer.listensynchronously invokes it. If the implementation ever reorders these calls,errorHandlerwould beundefinedand the test would silently hang on an unresolved promise rather than fail loudly. A short inline comment noting this ordering dependency, or a defensiveexpect(errorHandler).toBeDefined()before triggering it, would make the failure mode more diagnosable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server-elysia/src/elysia-server-provider.spec.ts` around lines 139 - 160, The test relies on server.once("error", ...) being called before server.listen(...) so errorHandler is set; add a defensive check (or short inline comment) in the test to make this dependency explicit and fail loudly if ordering changes: after mockServer.once.mockImplementation and before invoking mockServer.listen (or before calling provider.start()), assert that errorHandler is defined (e.g., expect(errorHandler).toBeDefined()) and/or add a brief comment referencing the ordering dependency; this targets the errorHandler captured in the it("should handle startup errors and release port") block and ensures the test won't hang silently if server.once is registered after listen.packages/server-core/src/auth/utils.spec.ts (1)
9-29: Consider adding an explicitundefinedNODE_ENV case.The suite covers
"development","test","production", and"", but the docstring onisDevEnvironment()emphasizes that undefinedNODE_ENVis the primary fail-closed scenario (deployments that forgot to set it).vi.stubEnv("NODE_ENV", undefined)(or deleting viadelete process.env.NODE_ENVinside the test) would pin down that behavior explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server-core/src/auth/utils.spec.ts` around lines 9 - 29, Add an explicit test to assert that isDevEnvironment() returns false when NODE_ENV is undefined: inside the existing "isDevEnvironment" describe block add a case that clears or unsets the env (e.g., use vi.stubEnv("NODE_ENV", undefined) or delete process.env.NODE_ENV) and expect(isDevEnvironment()).toBe(false); this pins down the fail-closed behavior for undefined NODE_ENV and uses the same test helpers (vi.stubEnv) already present in the suite..changeset/fix-dev-auth-bypass-node-env.md (1)
5-12: Consider mentioningtestin the headline for completeness.The title says "require explicit NODE_ENV=development" but the body (and implementation) also accepts
NODE_ENV=test. A small wording tweak would prevent confusion for consumers scanning the changelog.📝 Proposed wording
-fix(auth): require explicit NODE_ENV=development for dev auth bypass +fix(auth): require explicit NODE_ENV=development|test for dev auth bypass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/fix-dev-auth-bypass-node-env.md around lines 5 - 12, Update the changelog headline to mention both development and test environments so it matches the implementation: change the title in .changeset/fix-dev-auth-bypass-node-env.md from "require explicit NODE_ENV=development for dev auth bypass" to something like "require explicit NODE_ENV=development or NODE_ENV=test for dev auth bypass" and ensure any summary lines referencing isDevRequest() or the dev bypass behavior also mention "test" to avoid consumer confusion.packages/server-core/src/auth/utils.ts (1)
45-58: Consider aligningshouldEnableSwaggerUIwith the new helper.
packages/server-core/src/server/app-setup.tsstill uses a directprocess.env.NODE_ENV === "production"check with fail-open semantics (any non-production value enables Swagger UI). It's not strictly a security issue since Swagger exposure is less sensitive than auth bypass, but for consistency — and to avoid the same "undefined NODE_ENV on deployed server" footgun — consider switching it to!isDevEnvironment()-style gating, or at minimum a shared constant, so env checks across the package stay uniform.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server-core/src/auth/utils.ts` around lines 45 - 58, The server app-setup uses a direct process.env.NODE_ENV === "production" check for shouldEnableSwaggerUI which is inconsistent with the new isDevEnvironment()/isDevRequest() helper pattern; update shouldEnableSwaggerUI in packages/server-core/src/server/app-setup.ts to use the shared isDevEnvironment() (or the inverse !isDevEnvironment()) instead of comparing NODE_ENV directly so that Swagger gating follows the same environment logic as isDevRequest(); ensure you import isDevEnvironment from auth/utils and replace the existing production-check branch with a call to isDevEnvironment() (or a shared constant) to maintain uniform behavior across the package.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.changeset/fix-dev-auth-bypass-node-env.md:
- Around line 5-12: Update the changelog headline to mention both development
and test environments so it matches the implementation: change the title in
.changeset/fix-dev-auth-bypass-node-env.md from "require explicit
NODE_ENV=development for dev auth bypass" to something like "require explicit
NODE_ENV=development or NODE_ENV=test for dev auth bypass" and ensure any
summary lines referencing isDevRequest() or the dev bypass behavior also mention
"test" to avoid consumer confusion.
In `@packages/server-core/src/auth/utils.spec.ts`:
- Around line 9-29: Add an explicit test to assert that isDevEnvironment()
returns false when NODE_ENV is undefined: inside the existing "isDevEnvironment"
describe block add a case that clears or unsets the env (e.g., use
vi.stubEnv("NODE_ENV", undefined) or delete process.env.NODE_ENV) and
expect(isDevEnvironment()).toBe(false); this pins down the fail-closed behavior
for undefined NODE_ENV and uses the same test helpers (vi.stubEnv) already
present in the suite.
In `@packages/server-core/src/auth/utils.ts`:
- Around line 45-58: The server app-setup uses a direct process.env.NODE_ENV ===
"production" check for shouldEnableSwaggerUI which is inconsistent with the new
isDevEnvironment()/isDevRequest() helper pattern; update shouldEnableSwaggerUI
in packages/server-core/src/server/app-setup.ts to use the shared
isDevEnvironment() (or the inverse !isDevEnvironment()) instead of comparing
NODE_ENV directly so that Swagger gating follows the same environment logic as
isDevRequest(); ensure you import isDevEnvironment from auth/utils and replace
the existing production-check branch with a call to isDevEnvironment() (or a
shared constant) to maintain uniform behavior across the package.
In `@packages/server-elysia/src/elysia-server-provider.spec.ts`:
- Around line 22-32: The module-level mockServer object is shared across tests
causing state leakage; in the beforeEach where you reapply implementations for
listen/close/once, also explicitly reset mockServer properties (e.g., set
mockServer.listening = false or true as appropriate and reinitialize any added
methods) so each test starts with a fresh known state; update the beforeEach to
reassign mockServer.listening and any other mutable fields on the mockServer
used by tests (while keeping vi.mock("node:http") and the mocked
listen/close/once implementations intact).
- Around line 139-160: The test relies on server.once("error", ...) being called
before server.listen(...) so errorHandler is set; add a defensive check (or
short inline comment) in the test to make this dependency explicit and fail
loudly if ordering changes: after mockServer.once.mockImplementation and before
invoking mockServer.listen (or before calling provider.start()), assert that
errorHandler is defined (e.g., expect(errorHandler).toBeDefined()) and/or add a
brief comment referencing the ordering dependency; this targets the errorHandler
captured in the it("should handle startup errors and release port") block and
ensures the test won't hang silently if server.once is registered after listen.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fcbab152-366c-47a8-97b9-9b525ba07170
📒 Files selected for processing (6)
.changeset/fix-dev-auth-bypass-node-env.md.changeset/fix-elysia-server-provider-tests.mdpackages/server-core/src/auth/utils.spec.tspackages/server-core/src/auth/utils.tspackages/server-core/src/websocket/setup.tspackages/server-elysia/src/elysia-server-provider.spec.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server-elysia/src/elysia-server-provider.spec.ts (1)
139-160:⚠️ Potential issue | 🟡 MinorStartup-error test relies on call ordering in the implementation — add a guard assertion.
This test works only because
startServer()callsserver.once("error", reject)beforeserver.listen(...), soerrorHandleris captured prior to the synchronouslistenmock firing it. If the implementation is ever refactored to register the error listener afterlisten(or viaoninstead ofonce),errorHandlerwill beundefinedand theif (errorHandler)branch will silently no-op, causingprovider.start()to hang instead of rejecting — the test would then time out rather than fail cleanly.Consider asserting the handler was captured and/or the event name, so a regression surfaces as an explicit failure:
🛡️ Suggested hardening
mockServer.listen.mockImplementation(() => { // Simulate an error during listen by calling the error handler - if (errorHandler) { - errorHandler(new Error("Startup failed")); - } + if (!errorHandler) { + throw new Error("Test setup: error handler was not registered before listen()"); + } + errorHandler(new Error("Startup failed")); return mockServer; }); await expect(provider.start()).rejects.toThrow("Startup failed"); expect(portManager.releasePort).toHaveBeenCalledWith(3000); + expect(mockServer.once).toHaveBeenCalledWith("error", expect.any(Function));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server-elysia/src/elysia-server-provider.spec.ts` around lines 139 - 160, The test relies on capturing errorHandler before mockServer.listen fires; make this explicit by asserting the handler was registered before forcing the listen error: after mockServer.once registration logic (or before calling provider.start()), add an assertion like expect(errorHandler).toBeDefined() or verify mockServer.once was called with "error" so the test fails loudly if the implementation registers listeners after listen; alternatively, change the mockServer.listen implementation to throw or fail if errorHandler is undefined to ensure provider.start() doesn't hang. Ensure you reference the existing errorHandler variable, mockServer.once and mockServer.listen mocks, and the provider.start() call when adding the guard.
🧹 Nitpick comments (1)
packages/server-elysia/src/elysia-server-provider.spec.ts (1)
22-32: Optional: resetmockServer.listeningper test and/or localize the mock.
mockServeris module-scoped withlistening: truehardcoded.vi.clearAllMocks()inafterEachclears call history but does not reset property values or re-apply implementations — you already re-seedlisten/close/onceinbeforeEach, butlisteningis never reset. Today no test mutates it, so it's harmless, but if a future test togglesmockServer.listening = false(e.g., to cover the "server not listening, skip close" branch instopServer), state will leak across tests in this file. Consider resetting it inbeforeEach:♻️ Suggested tweak
beforeEach(() => { vi.spyOn(appFactory, "createApp").mockResolvedValue({ app: mockApp } as any); provider = new ElysiaServerProvider(mockDeps, { port: 3000 }); // Reset mock server behavior for each test + mockServer.listening = true; mockServer.listen.mockImplementation((_port, _hostname, callback) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server-elysia/src/elysia-server-provider.spec.ts` around lines 22 - 32, The module-scoped mockServer has listening: true that isn't reset between tests; update the test setup so mockServer.listening is re-initialized each test (either set mockServer.listening = true in beforeEach or create the mock object inside beforeEach and have vi.mock("node:http", ...) return a fresh instance) so state doesn't leak across tests—ensure the mocked createServer implementation used by the tests (the vi.mock factory and the mockServer referenced by beforeEach/afterEach and any tests calling stopServer) always gets a fresh listening value per test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/server-elysia/src/elysia-server-provider.spec.ts`:
- Around line 139-160: The test relies on capturing errorHandler before
mockServer.listen fires; make this explicit by asserting the handler was
registered before forcing the listen error: after mockServer.once registration
logic (or before calling provider.start()), add an assertion like
expect(errorHandler).toBeDefined() or verify mockServer.once was called with
"error" so the test fails loudly if the implementation registers listeners after
listen; alternatively, change the mockServer.listen implementation to throw or
fail if errorHandler is undefined to ensure provider.start() doesn't hang.
Ensure you reference the existing errorHandler variable, mockServer.once and
mockServer.listen mocks, and the provider.start() call when adding the guard.
---
Nitpick comments:
In `@packages/server-elysia/src/elysia-server-provider.spec.ts`:
- Around line 22-32: The module-scoped mockServer has listening: true that isn't
reset between tests; update the test setup so mockServer.listening is
re-initialized each test (either set mockServer.listening = true in beforeEach
or create the mock object inside beforeEach and have vi.mock("node:http", ...)
return a fresh instance) so state doesn't leak across tests—ensure the mocked
createServer implementation used by the tests (the vi.mock factory and the
mockServer referenced by beforeEach/afterEach and any tests calling stopServer)
always gets a fresh listening value per test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5bdc7a9d-38fa-424f-954d-9c87740aa83d
📒 Files selected for processing (2)
packages/server-core/src/auth/utils.tspackages/server-elysia/src/elysia-server-provider.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server-core/src/auth/utils.ts
1 participant
Summary
Fixes #1204 — all 6 tests in
elysia-server-provider.spec.tswere failing because the test mocks targeted the oldapp.listen()API, butstartServer()was refactored to usehttp.createServer()+server.listen().Changes
vi.mock('node:http')to mockcreateServerinstead of relying onmockApp.listenbeforeEachto configure mock server behavior (listen/close/once callbacks)fetchto mock app (required by the HTTP request handler)createServerandserver.listencallsserver.close()is calledserver.once('error')handlerTesting
All 6 tests pass:
Closes #1204
Summary by cubic
Updates Elysia server tests to mock
node:http(createServer/server.listen) after the startServer refactor, restoring all six cases (fixes #1204). Secures the dev auth bypass in@voltagent/server-coreby requiringNODE_ENV=developmentortestonly (fail-closed), applies the same check to WebSocket paths, and cleans up lint errors.@voltagent/server-elysia: mocknode:http; assertcreateServer,server.listen, andserver.close; simulate startup errors viaserver.once('error').@voltagent/server-core: addisDevEnvironment(); updateisDevRequest()and WS dev bypass to use it; treat empty/undefinedNODE_ENVas production; add unit tests.Written for commit bcb54c8. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests